prometheus: Add settings to prometheus plugin #87
Conversation
This patch adds settings to the prometheus plugin allowing the users to configure the service address, timespan, enable metrics for each cluster. Signed-off-by: yolossn <[email protected]>
|
Is there an issue to link this to? Can you please add a "how to test" section to the PR description? |
| @@ -12,6 +55,7 @@ export interface ChartProps { | |||
| dataProcessor: (data: any) => any[]; | |||
| }>; | |||
| fetchMetrics: (query: object) => Promise<any>; | |||
| interval: string; | |||
There was a problem hiding this comment.
Can you please document these fields?
| * @param {Function} props.onDataChange - The function to call when the data changes. | ||
| * @returns {JSX.Element} The Settings component. | ||
| */ | ||
| export function Settings(props) { |
There was a problem hiding this comment.
Can you please add a SettingsProps interface with the docs in there?
| @@ -0,0 +1,170 @@ | |||
| import { NameValueTable } from '@kinvolk/headlamp-plugin/lib/components/common'; | |||
There was a problem hiding this comment.
Component filenames should usually be capital named the same as the component Settings.tsx. Putting them in a folder (Settings/Settings.stories.tsx, Settings/Settings.tsx etc) makes it a bit easier as the number of components grows or there begins to be a lot of files for that component.
| * @param {string} interval - The time interval (e.g., '10m', '1h', '24h', 'week'). | ||
| * @returns {Object} An object containing the 'from' timestamp, 'to' timestamp, and 'step' in seconds. | ||
| */ | ||
| export function getTimeRange(interval: string): { from: number; to: number; step: number } { |
There was a problem hiding this comment.
Is there somewhere this is specified? A URL to some docs or code perhaps?
| * @returns {Object} An object containing the 'from' timestamp, 'to' timestamp, and 'step' in seconds. | ||
| */ | ||
| export function getTimeRange(interval: string): { from: number; to: number; step: number } { | ||
| const now = Math.floor(Date.now() / 1000); |
There was a problem hiding this comment.
This function could use some tests.
| export function CPUChart(props: { query: string; prometheusPrefix: string; autoRefresh: boolean }) { | ||
| const xTickFormatter = createTickTimestampFormatter(); | ||
| export function CPUChart(props: { | ||
| query: string; |
There was a problem hiding this comment.
Can you please document these fields? CPUChart could be in a CPUChart.tsx, because in general one file one component is better.
Can you please create a CPUChartProps interface for these fields?
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "prometheus", | |||
| "name": "@headlamp-k8s/prometheus", | |||
There was a problem hiding this comment.
This will affect how this is installed.
How to handle this? At minimum a note in the release notes (or in the README?) should warn people to remove their old prometheus plugin, what the old one is called and what the new folder name is called.
This could also affect settings/permissions for running plugins.
There was a problem hiding this comment.
I think the app/ bundling will need to be changed to get the new plugin name too?
|
With these changes will it work with Azure Monitor? I started an issue for this here: I think these are the related issues for this PR? |
|
This comment was made in this issue: #24 (comment)
Before closing #24 that should addressed IMHO. |
This patch adds settings to the prometheus plugin
allowing the users to configure the service
address, timespan, enable metrics for each
cluster.
Fixes:
#24
Includes the fix suggested here:
#37 (comment)
Steps to Test:
prom_settingsbranch.